Need advice on this TODO

Returning the first error is fine, but returning all the errors would be more consistent with the rest of the validation APIs.

Re your second question: yes, validation does some additional steps (e.g., handling for templates). If you look at validate and validate_policies here you'll see that there are several pre-processing steps before we get to typecheck_policy (which is what calls typecheck_by_request_env). I think it'd be ideal to add you changes to the main validation path (if possible).

I'm not sure how to add it to the validation path without it getting ugly- it would have to return both the current result and an optional entity manifest.
I'll keep this as-is for now.

Ended up running full validation first before this code path, giving a good error message

Returning the first error is fine, but returning all the errors would be more consistent with the rest of the validation APIs.

Re your second question: yes, validation does some additional steps (e.g., handling for templates). If you look at validate and validate_policies here you'll see that there are several pre-processing steps before we get to typecheck_policy (which is what calls typecheck_by_request_env). I think it'd be ideal to add you changes to the main validation path (if possible).

I think this will end up exporting cedar_policy_core::ast::EntityUID from cedar_policy. Not a blocking issue for an experimental feature, but we'll need to introduce some more wrapper type in cedar_policy if we want callers to be able to use this directly as a cedar_policy::EntityUID. Same comment for Var variant.

Do you mean that this should use the existing wrapper EntityUid?
That's a little unfortunate since I can't import it from the validator crate.

Right, Callers should be able to use it as they would the existing EntityUid wrapper. It does get a annoying since you'll end up wanting a wrapper for most of your structs, but we don't want consumers of cedar-policy to ever import cedar-policy-core

The other option would be using the opaque inner struct pattern we use for error messages if users don't need to look inside the Literal.

That makes sense. I think people should be able to get the literal, so for now I'll add a TODO to implement wrappers for everything in api.rs

This assumption might not hold. Check behavior on undefined action and entity types

Returning the full list of errors would avoid any uncertainty here

That's a good idea
How should I implement Diagnostic for multiple validation errors? Should it print out the first one? What about when there are no errors?

You can take a look at ParseErrors in Core for precedent. To the first question, at least in that case we chose to just do the first one. To the second question, we used NonEmpty to structurally guarantee we can't have an empty vector.

Thanks to the pointer to ParseErrors
I'm still having trouble finding the case where there are no errors reported- could you point me in the right direction? What are undefined action and entity types?

One option could be to

1. Run full strict mode validation, getting a proper api::ValidationResult
2. Run typecheck_by_request_env to get the type-annotated policies, using panic if we encounter a type error

Another option would be to embed entity manifest computation into the typechecker to make this more efficient. But this seems kind of ugly.

I'm still having trouble finding the case where there are no errors reported- could you point me in the right direction? What are undefined action and entity types?

In my case, all of the request environments are from the schema cross product so we should be okay I think.
But either way, I've rewritten the code to do full validation first, then get the type-annotated policies.